Hi, I see that there some CI tests are failing, seems not able to call ecall_handle_cmd_exec_app_func()?

BTW, what is the purpose of using idx rather than raw pointer for module_inst and other data? Is it for easier management with the help of the newly introduced pointer manager?

BTW, what is the purpose of using idx rather than raw pointer for module_inst and other data? Is it for easier management with the help of the newly introduced pointer manager?

Use raw pointer is very dangerous, since it can point to in-Enclave memory, it's hard to check pointed memory is a valid context content or invalid sensitive in-Enclave data. So use indexes to replace the raw pointers when passing enclave_module and wasm_module_inst across the SGX boundary. This method is decribed in TeeRex, and at the end of section 5.3

The developers of the Rust SGX SDK acknowledged the problem and promptly updated their code. Akin to our suggestions to the developers, the enclave code now utilizes session identifiers instead of pointers to identify TLS sessions; similar to using file descriptors on Unix-like systems. Upon session creation in tls_client_new, the pointer to the TLS session object is now inserted into a hashmap, which is then used to map the identifier in subsequent ECALLs. Hence, no pointers are passed on the host-to-enclave boundary. This drastically reduces the attack surface of the enclave and eradicates both the vulnerability pattern Returning pointers to enclave memory (P2) and Pointers to Overlapping Memory (P3).

Thanks! That makes sense

Hi, I see that there some CI tests are failing, seems not able to call ecall_handle_cmd_exec_app_func()?

Do you means CI tests successfully call ecall_handle_cmd_exec_app_func, but crash or return error inside this function? Or CI tests can't successfully call ecall_handle_cmd_exec_app_func？If it's the first situation, maybe the check added by myself in ecall_handle_cmd_exec_app_func result in checking fault

Thanks! That makes sense

This part of code maybe can be optimized. I think it's better to avoid using lock

Hi, I see that there some CI tests are failing, seems not able to call ecall_handle_cmd_exec_app_func()?

Do you means CI tests successfully call ecall_handle_cmd_exec_app_func, but crash or return error inside this function? Or CI tests can't successfully call ecall_handle_cmd_exec_app_func？If it's the first situation, maybe the check added by myself in ecall_handle_cmd_exec_app_func result in checking fault

Can you provide more detail about it? Then I can figure out why it failed

Do you means CI tests successfully call ecall_handle_cmd_exec_app_func, but crash or return error inside this function? Or CI tests can't successfully call ecall_handle_cmd_exec_app_func？If it's the first situation, maybe the check added by myself in ecall_handle_cmd_exec_app_func result in checking fault

https://github.com/bytecodealliance/wasm-micro-runtime/actions/runs/5548246017/job/15029123379?pr=2357

Some spec tests are failing, output is something like:

Starting interpreter for module '/tmp/tmp9j2olynn.aot'
Running: ../../../product-mini/platforms/linux-sgx/enclave-sample/iwasm --heap-size=0 -v=0 --repl /tmp/tmp9j2olynn.aot
Testing(return) i32_add  = 0x102:i32
THE FINAL EXCEPTION IS out should be in a form likes numbers:type, but Call ecall_handle_cmd_exec_app_func() failed.

You can run a hello world aot file with similar cmd option to reproduce it to double-check whether it's due to the newly added check

Can you provide more detail about it? Then I can figure out why it failed

On your local machine, you can also run spec tests to check:

# assume you install the SDK in /opt/intel/sgxsdk/
source /opt/intel/sgxsdk/environment
# in WAMR root directory
cd tests/wamr-test-suites
# the shell script will automatically compile needed component and run spec test on sgx iwasm
./test_wamr.sh -b -x -t aot -s spec -p -P

Do you means CI tests successfully call ecall_handle_cmd_exec_app_func, but crash or return error inside this function? Or CI tests can't successfully call ecall_handle_cmd_exec_app_func？If it's the first situation, maybe the check added by myself in ecall_handle_cmd_exec_app_func result in checking fault

https://github.com/bytecodealliance/wasm-micro-runtime/actions/runs/5548246017/job/15029123379?pr=2357

Some spec tests are failing, output is something like:

Starting interpreter for module '/tmp/tmp9j2olynn.aot'
Running: ../../../product-mini/platforms/linux-sgx/enclave-sample/iwasm --heap-size=0 -v=0 --repl /tmp/tmp9j2olynn.aot
Testing(return) i32_add  = 0x102:i32
THE FINAL EXCEPTION IS out should be in a form likes numbers:type, but Call ecall_handle_cmd_exec_app_func() failed.

You can run a hello world aot file with similar cmd option to reproduce it to double-check whether it's due to the newly added check

Ok, I'll try aot mode. I currently is preparing thesis proposal, so maybe late to fix it.

Can you provide more detail about it? Then I can figure out why it failed

On your local machine, you can also run spec tests to check:

# assume you install the SDK in /opt/intel/sgxsdk/
source /opt/intel/sgxsdk/environment
# in WAMR root directory
cd tests/wamr-test-suites
# the shell script will automatically compile needed component and run spec test on sgx iwasm
./test_wamr.sh -b -x -t aot -s spec -p -P

Thanks

No hurry, good luck with your thesis!

No hurry, good luck with your thesis!

Thanks!

@TianlongLiang Hello, I fixed the parameters check in 1306d68, and all CI tests passed

LGTM

CI tests failed again. What is nuttx. How can I reproduce this problem in my local machine

CI tests failed again. What is nuttx. How can I reproduce this problem in my local machine

Nuttx CI issue was fixed with PR #2414, could you rebase again?

CI tests failed again. What is nuttx. How can I reproduce this problem in my local machine

Nuttx CI issue was fixed with PR #2414, could you rebase again?

OK, I try it

@LeoneChen Many thanks for the fixing, it really improves the security of WAMR linux-sgx. We are preparing to release 1.2.3, since the changes of this PR are a little big and complex, we (and some customers) need to more time to test, I think we had better merge this PR after the new release is created, and currently we can create another PR to fix the issue of wait_map = NULL; in wasm_shared_memory_destroy. Is it OK for you?

@LeoneChen Many thanks for the fixing, it really improves the security of WAMR linux-sgx. We are preparing to release 1.2.3, since the changes of this PR are a little big and complex, we (and some customers) need to more time to test, I think we had better merge this PR after the new release is created, and currently we can create another PR to fix the issue of wait_map = NULL; in wasm_shared_memory_destroy. Is it OK for you?

Yes, it's OK. Fix wasm_shared_memory_destroy is urgent.

This PR is complex and needs more review, and I also need to think about how to write back content to u_app_argv.

Fix it by yourself or I create a new PR?

Fix it by yourself or I create a new PR?

Let me fix it by myself😊

OK

Hi, I checked again, wasm_application_execute_main puts the return value into *(int *)argv, here should be *(int )(app_argv + 1), see: https://github.com/bytecodealliance/wasm-micro-runtime/blob/main/core/iwasm/include/wasm_export.h#L836-L838
So my understanding is that we had better write(int *)(app_argv + 1) back into *(int *)u_app_argv?

wasm_application_execute_main call execute_main, which write back int type return value that use char** type argv as int *.

I think it's not a good code practice...

And in current situation, app_argv + 1 is malloc-ed in Enclave with memory clone (for security), when execute_main store int type return value to app_argv + 1, it will cause address of malloc-ed char string be overwrite, and then we can't free this deep copied char string.

Maybe can use stack memory

Hi, I checked again, wasm_application_execute_main puts the return value into *(int *)argv, here should be *(int )(app_argv + 1), see: https://github.com/bytecodealliance/wasm-micro-runtime/blob/main/core/iwasm/include/wasm_export.h#L836-L838
So my understanding is that we had better write(int *)(app_argv + 1) back into *(int *)u_app_argv?

wasm_application_execute_main call execute_main, which write back int type return value that use char** type argv as int *.

I think it's not a good code practice...

And in current situation, app_argv + 1 is malloc-ed in Enclave with memory clone (for security), when execute_main store int type return value to app_argv + 1, it will cause address of malloc-ed char string be overwrite, and then we can't free this deep copied char string.

Maybe can use stack memory

Yes, how about adding an extra argument in ecall_handle_cmd_exec_app_main to output the result? e.g.

public bool ecall_handle_cmd_exec_app_main(
        uint16_t wasm_module_inst_idx,
        [in, count=app_argc] char **u_app_argv,
        uint32_t app_argc,
        [out, count=1] int *ret_main
      );

Fix it by yourself or I create a new PR?

@LeoneChen I found more similar issues in Enclave, and uploaded PR #2416 to fix them, could you help review it? Thanks.

Fix it by yourself or I create a new PR?

@LeoneChen I found more similar issues in Enclave, and uploaded PR #2416 to fix them, could you help review it? Thanks.

OK

@LeoneChen We've created a new PR to enhance the enclave. Please take a look and share your comments.

@lum1n0us OK, let me fuzz the patched version (#4828) again.